CSHARP-5672: Support sorting by value in PushEach operation #1748
Conversation
| _direction = direction; | ||
| } | ||
|
|
||
| public override BsonDocument Render(RenderArgs<TDocument> args) |
There was a problem hiding this comment.
Do we want a Direction property like DirectionalSortDefinition has?
There was a problem hiding this comment.
We currently have no use for that property, so I decided to omit it for now to avoid introducing unused code.
| { | ||
| SortDirection.Ascending => 1, | ||
| SortDirection.Descending => -1, | ||
| _ => throw new InvalidOperationException("Unknown value for " + typeof(SortDirection) + ".") |
There was a problem hiding this comment.
I would have done this:
_ => throw new InvalidOperationException($"Invalid sort direction: {_direction}.")
but I suppose then we might want to change DirectionalSortDefinition also...
| _ => throw new InvalidOperationException("Unknown value for " + typeof(SortDirection) + ".") | ||
| }; | ||
|
|
||
| return new BsonDocument("direction", value); |
There was a problem hiding this comment.
When we use "magic" values like this we usually enclose them in angle brackets to make it visually apparent that something is special about this value:
return new BsonDocument("<direction>", value);
We only have a field name here because Render is already declared to return BsonDocument and changing it to return BsonValue would be a breaking change.
There was a problem hiding this comment.
So what happens in other cases (which are not UpdateDefinitionBuilder), will it be rendered as
sort: { direction: ... } and do we want that?
We could add
internal virtual BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => Render(args);
to SortDefinition, and refactor Render to return BsonValue in 4.0.
There was a problem hiding this comment.
Nice, I like this! I thought of something similar but thought adding something to SortDefinition would be a breaking change but I suppose adding an internal virtual method is not? Learning some library maintainer tricks :)
There was a problem hiding this comment.
With this updated approach, I am also thinking of throwing an exception when a call to Render is made on a ValueDirectionalSortDefinition. A call to render for ValueDirectionalSortDefinition is suggesting someone is trying to use this where a field-based sort is expected which is invalid.
| var newArgs = args.WithNewDocumentType((IBsonSerializer<TItem>)itemSerializer); | ||
|
|
||
| var renderedSort = _sort is NoFieldDirectionalSortDefinition<TItem> | ||
| ? _sort.Render(newArgs)["direction"] |
There was a problem hiding this comment.
? _sort.Render(newArgs)["<direction>"]
Note the added angle brackets.
|
|
||
| public override BsonDocument Render(RenderArgs<TDocument> args) | ||
| { | ||
| BsonValue value = _direction switch |
There was a problem hiding this comment.
We do the SortDirection to BsonValue conversion in 2 different places already.
Consider extorting this to internal SortDirectionExtensions or similar?
| _ => throw new InvalidOperationException("Unknown value for " + typeof(SortDirection) + ".") | ||
| }; | ||
|
|
||
| return new BsonDocument("direction", value); |
There was a problem hiding this comment.
So what happens in other cases (which are not UpdateDefinitionBuilder), will it be rendered as
sort: { direction: ... } and do we want that?
We could add
internal virtual BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => Render(args);
to SortDefinition, and refactor Render to return BsonValue in 4.0.
adelinowona
force-pushed
the
csharp5672
branch
from
a47ba07 to
121021d
Compare
No description provided.